-
Notifications
You must be signed in to change notification settings - Fork 94
feat(android): implement deep link support for Android #19581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (54)
|
caybro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general
157cd47 to
c5603fa
Compare
caybro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
c5603fa to
af91799
Compare
|
Fails to build DOtherside with: |
4e80a08 to
4f8fac9
Compare
4f8fac9 to
50dce40
Compare
micieslak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could do that without altering StatusQActivity, in a bit more separated way. Ideally also without DOtherSide. We have https://github.com/status-im/MobileUI that I added some time ago to handle interoperability with native mobile APIs. Not sure if applicable here but probably worth checking.
| // Called from Qt via JNI when main window is visible | ||
| public static void hideSplashScreen() { | ||
| splashShouldHide.set(true); | ||
| userLoggedIn.set(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to track the logged in state it's probably not the best to do it here when the splash screen gets hidden, right? We could add a flag in the activity that's driven by qml/c++ to save the login state.
alexjba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't involve DOtherSide at all. I still hope we'll drop it in favor of seaqt and it would be nice to keep it clean.
An alternative would be to move the c++ implementation in StatusQ - for status specific, or MobileUI for generic android stuff.
Fixes #19562 Adds the needed manifest info to tell Android which links we supports. Then adds the Java code necessary to handle it. In this case, we listen to the Intent and if there is a URL, we emit an event that can be listened to in C++, which gets sent to the url_manager in Nim. Then we reuse the same code we already had.
50dce40 to
19a6053
Compare
|
@alexjba I tried to move the C++ code to MobileUI, but then it doesn't get found by the Java code: I think there is some linking change needed so that MobileUI functions can be called from Java. However, I don't have the knowledge or the time currently, with the fact that we want to cut this Friday and I currently use the Docker container to build Android locally, so it's very slow. I also renamed the |
What does the PR do
Fixes #19562
Adds the needed manifest info to tell Android which links we supports.
Then adds the Java code necessary to handle it. In this case, we listen to the Intent and if there is a URL, we emit an event that can be listened to in C++, which gets sent to the url_manager in Nim. Then we reuse the same code we already had.
Affected areas
Architecture compliance
My PR is consistent with this document: QML Architecture Guidelines
Screencapture of the functionality
andrdoid-deep-links.webm
Impact on end user
Makes deep linking to the Status app work.
Reading the docs, if two apps support the same scheme, a drawer will be shown asking which app they choose to open, so all good.
How to test
Risk
Low